crates/dojo/core/src/model/metadata.cairo (2)

12-12: Ohayo! The metadata_hash addition looks solid, sensei!

The new metadata_hash field is a well-thought-out solution that enables efficient change detection for metadata updates. This aligns perfectly with the PR's objective of uploading metadata only when necessary, especially after failed deployments.

This approach provides several benefits:

• Efficient change detection through hash comparison
• Prevents unnecessary uploads when metadata hasn't changed
• Enables retry mechanisms after failed deployments

---

12-12: Consider adding documentation for the metadata_hash field, sensei!

While the implementation is solid, adding documentation comments would help other developers understand the purpose and usage of this new field.

 pub struct ResourceMetadata {
     #[key]
     pub resource_id: felt252,
     pub metadata_uri: ByteArray,
+    /// Hash of the metadata content, used to detect changes and prevent unnecessary uploads
     pub metadata_hash: felt252
 }

examples/spawn-and-move/dojo_dev.toml (2)

18-21: Consider enhancing the event documentation, sensei!

The event description could be more detailed to specify:

• What data is emitted when a player moves
• When exactly this event is triggered
• Who can listen to this event

---

22-25: Please elaborate on the contract's actions, sensei!

The description "set of actions for a player" could be more specific about:

• What actions are available
• Any constraints or prerequisites for these actions
• How these actions relate to the models and events

crates/dojo/world/src/metadata/ipfs.rs (1)

17-22: Consider adding retry mechanism for IPFS client initialization

The client initialization looks good, but IPFS services can be occasionally unstable. Consider adding retry logic for better reliability.

Example implementation:

use tokio::time::{sleep, Duration};

pub fn new() -> Result<Self> {
    let max_retries = 3;
    let mut attempt = 0;
    
    loop {
        match ipfs_api_backend_hyper::IpfsClient::from_str(IPFS_CLIENT_URL) {
            Ok(client) => return Ok(Self {
                client: client.with_credentials(IPFS_USERNAME, IPFS_PASSWORD),
            }),
            Err(e) if attempt < max_retries => {
                attempt += 1;
                sleep(Duration::from_secs(1)).await;
                continue;
            }
            Err(e) => return Err(e.into()),
        }
    }
}

crates/dojo/world/src/remote/mod.rs (1)

26-27: Consider adding tests for metadata_hash initialization and updates

Ohayo sensei! While the field addition is solid, we should ensure it's properly tested. Consider adding test cases to verify:

1. Default initialization of metadata_hash
2. Metadata hash updates through the WorldRemote interface

Here's a suggested test case:

#[test]
fn test_world_metadata_hash() {
    let world_remote = WorldRemote::default();
    assert_eq!(world_remote.metadata_hash, Felt::ZERO, "Default metadata hash should be zero");
}

crates/dojo/core-cairo-test/src/tests/world/metadata.cairo (1)

Line range hint 1-120: Consider adding edge cases for metadata_hash validation, sensei!

While the current test coverage is good, consider adding test cases for:

1. Zero hash values
2. Maximum u32 hash values
3. Hash collisions between different metadata

Here's a suggested test case:

#[test]
fn test_set_metadata_edge_cases() {
    let world = deploy_world();
    let world = world.dispatcher;

    // Test zero hash
    let metadata_zero = ResourceMetadata {
        resource_id: 0,
        metadata_uri: format!("ipfs:zero"),
        metadata_hash: 0
    };
    world.set_metadata(metadata_zero.clone());
    assert(world.metadata(0) == metadata_zero, 'invalid zero hash');

    // Test max hash
    let metadata_max = ResourceMetadata {
        resource_id: 0,
        metadata_uri: format!("ipfs:max"),
        metadata_hash: 0xffffffff
    };
    world.set_metadata(metadata_max.clone());
    assert(world.metadata(0) == metadata_max, 'invalid max hash');
}

crates/dojo/world/src/uri.rs (2)

21-35: Excellent Hash implementation, sensei!

The Hash implementation is well-structured and correctly handles all Uri variants. This aligns perfectly with the PR's objective of tracking metadata changes.

Consider adding test cases for the Hash implementation to ensure consistent behavior across variants. Here's a suggested test:

#[test]
fn test_uri_hash() {
    use std::collections::hash_map::DefaultHasher;
    use std::hash::{Hash, Hasher};

    fn calculate_hash<T: Hash>(t: &T) -> u64 {
        let mut s = DefaultHasher::new();
        t.hash(&mut s);
        s.finish()
    }

    let uri1 = Uri::Ipfs("ipfs://QmYwAPJzv5CZsnA625s3Xf2nemtYgPpHdWEz79ojWnPbdG".to_string());
    let uri2 = Uri::Ipfs("ipfs://QmYwAPJzv5CZsnA625s3Xf2nemtYgPpHdWEz79ojWnPbdG".to_string());
    let uri3 = Uri::Ipfs("ipfs://different".to_string());

    assert_eq!(calculate_hash(&uri1), calculate_hash(&uri2));
    assert_ne!(calculate_hash(&uri1), calculate_hash(&uri3));
}

---

Line range hint 1-69: Ohayo! Great architectural contribution, sensei!

These implementations provide essential building blocks for the PR's objective of uploading metadata only when changed. The Hash implementation enables efficient change detection, while the Display implementation supports better debugging and logging capabilities.

This lays a solid foundation for metadata change tracking. Consider documenting these new capabilities in the module-level documentation to help other developers understand how to leverage them for metadata comparison.

crates/dojo/world/src/diff/resource.rs (2)

116-123: Ohayo! Add documentation for metadata hash semantics

The implementation looks solid, sensei! However, it would be helpful to document why Felt::ZERO is used for newly created resources and how this relates to the metadata upload process.

Consider adding documentation like this:

    /// Returns the current metadata hash of the resource.
+   ///
+   /// For newly created resources that haven't been deployed yet, returns `Felt::ZERO`
+   /// as no metadata has been uploaded. For existing resources, returns the hash of
+   /// their current metadata from the remote state.
    pub fn metadata_hash(&self) -> Felt {

---

116-123: Implementation aligns well with existing patterns

The implementation follows the established pattern of other methods in the enum, correctly handles all variants, and integrates smoothly with the remote metadata functionality. This change supports the PR objective of tracking metadata changes.

Consider adding a helper method to check if metadata needs updating by comparing the local metadata hash with this remote hash. This would support the PR's goal of uploading "only if changed".

crates/sozo/ops/src/tests/migration.rs (2)

89-111: Add documentation to the helper function, sensei!

While the function is well-structured, it would benefit from documentation explaining:

• The purpose of the function
• Parameter descriptions
• The meaning of the checker function
• Return value implications

Here's a suggested documentation block:

+/// Validates resource metadata in a WorldDiff against expected criteria
+/// 
+/// # Arguments
+/// * `diff` - The WorldDiff containing resources to check
+/// * `resources` - Optional vector of ResourceConfig to validate
+/// * `expected_count` - Expected number of resources
+/// * `checker` - Function to validate metadata hash values
+/// 
+/// # Panics
+/// Panics if assertions fail for resource count or metadata hash validation

---

113-150: Excellent test implementation, sensei!

The test thoroughly verifies metadata state transitions and covers all resource types. A few observations:

1. Good use of helper functions is_set and is_not_set
2. Comprehensive verification of world and resource metadata
3. Clear comments explaining the test flow

One suggestion: Consider adding a test case for error conditions when metadata upload fails.

Here's a suggested additional test case:

#[tokio::test(flavor = "multi_thread")]
#[katana_runner::test(accounts = 10)]
async fn upload_metadata_handles_errors(sequencer: &RunnerCtx) {
    // Test implementation for error conditions
    // This would help verify the error handling in the migration process
}

crates/dojo/world/src/diff/compare.rs (1)

Line range hint 17-27: Ohayo! Consider adding metadata_hash comparison in ContractLocal implementation

The comparison logic only checks class_hash but doesn't account for metadata changes. Based on the PR objectives to handle metadata updates, we should also compare metadata_hash.

Here's a suggested implementation:

 impl ComparableResource for ContractLocal {
     fn compare(self, remote: ResourceRemote) -> ResourceDiff {
         let remote_contract = remote.as_contract_or_panic();
 
-        if self.common.class_hash == remote_contract.common.current_class_hash() {
+        if self.common.class_hash == remote_contract.common.current_class_hash() 
+            && self.metadata_hash == remote_contract.common.metadata_hash {
             ResourceDiff::Synced(ResourceLocal::Contract(self), remote)
         } else {
             ResourceDiff::Updated(ResourceLocal::Contract(self), remote)
         }
     }
 }

crates/dojo/world/src/config/profile_config.rs (2)

21-23: Ohayo! Well-structured metadata fields, sensei!

The new optional fields for models, contracts, and events are well-organized and follow a consistent pattern. This structure aligns perfectly with the PR's objective to improve metadata handling.

Consider documenting these fields with rustdoc comments to help future maintainers understand their purpose and usage patterns.

---

141-154: Ohayo! Consider adding edge cases to the test suite, sensei.

While the current test coverage is good, consider adding these scenarios:

1. Empty vectors for models/contracts/events
2. Missing optional fields (description, icon_uri)
3. Invalid URI formats
4. Duplicate tags within the same resource type

This will help ensure robust handling of various input scenarios.

Here's a suggested test case to add:

#[test]
fn test_profile_config_edge_cases() {
    let content = r#"
    [world]
    name = "test"
    seed = "abcd"

    [namespace]
    default = "test"

    [[models]]
    tag = "empty-model"

    [[contracts]]
    tag = "invalid-contract"
    icon_uri = "not-a-valid-uri"
    "#;

    let config = toml::from_str::<ProfileConfig>(content).unwrap();
    
    // Test empty models
    assert!(config.models.is_some());
    let models = config.models.unwrap();
    assert_eq!(models[0].description, None);
    assert_eq!(models[0].icon_uri, None);

    // Test invalid URI
    assert!(config.contracts.is_some());
    let contracts = config.contracts.unwrap();
    assert!(contracts[0].icon_uri.is_none());
}

Also applies to: 212-232

crates/dojo/world/src/diff/mod.rs (2)

82-82: Consider documenting the ZERO initialization significance

The initialization of metadata_hash to Felt::ZERO in from_local implies this is a new world without existing metadata, but this significance isn't documented.

-                metadata_hash: Felt::ZERO,
+                // Initialize to ZERO for new worlds without existing metadata
+                metadata_hash: Felt::ZERO,

---

Line range hint 32-123: Ohayo sensei! Clean and focused metadata tracking implementation

The approach of using a dedicated metadata_hash field in WorldStatusInfo is elegant and maintainable. It:

1. Cleanly separates metadata state tracking from other world state
2. Enables efficient metadata change detection
3. Maintains backward compatibility with existing world diff logic

Consider documenting this design decision in the module-level documentation to help future maintainers understand the rationale.

crates/dojo/world/src/remote/events_to_remote.rs (2)

254-264: LGTM with a minor enhancement suggestion

Ohayo sensei! The MetadataUpdate event handling looks solid with proper separation between world and resource metadata updates.

Consider adding trace logging for world metadata updates similar to resource updates for consistency.

 if e.resource == Felt::ZERO {
     self.metadata_hash = e.hash;
+    trace!("World metadata updated with hash: {:#066x}", e.hash);
 } else {

---

532-563: Add test coverage for world metadata updates

Ohayo sensei! While the current test thoroughly covers resource metadata updates, we should also add test coverage for world metadata updates (when resource == Felt::ZERO).

#[tokio::test]
async fn test_world_metadata_updated_event() {
    let mut world_remote = WorldRemote::default();
    
    let event = WorldEvent::MetadataUpdate(world::MetadataUpdate {
        resource: Felt::ZERO,
        uri: ByteArray::from_string("ipfs://world").unwrap(),
        hash: Felt::THREE,
    });

    world_remote.match_event(event).unwrap();
    assert_eq!(world_remote.metadata_hash, Felt::THREE);
}

crates/dojo/core/src/world/world_contract.cairo (1)

114-115: Consider adding documentation for the hash field usage, sensei!

While the implementation is correct, it would be helpful to add documentation explaining:

• The hash calculation method
• When the hash changes
• How to handle hash conflicts

Add documentation above the MetadataUpdate event:

+    /// Emitted when a resource's metadata is updated
+    /// * `resource` - The resource identifier
+    /// * `uri` - The metadata URI
+    /// * `hash` - Hash of the metadata content for change detection
     #[derive(Drop, starknet::Event)]
     pub struct MetadataUpdate {

Also applies to: 360-364

crates/dojo/world/src/metadata/mod.rs (3)

46-65: Refactor to eliminate code duplication between implementations

Ohayo, sensei! The upload methods in both WorldMetadata and ResourceMetadata share similar logic for handling metadata uploads. To adhere to the DRY (Don't Repeat Yourself) principle, consider refactoring the common code into a helper function or a generic implementation. This will enhance maintainability and readability.

Here's how you might refactor the code:

1. Create a generic helper function:

   async fn upload_metadata<T>(meta: &mut T) -> Result<String>
   where
       T: Clone + Serialize + HasIconUri + HasCoverUri,
   {
       let ipfs_client = IpfsClient::new()?;
   
       if let Some(Uri::File(icon)) = meta.icon_uri() {
           let icon_data = std::fs::read(icon)?;
           meta.set_icon_uri(Some(Uri::Ipfs(ipfs_client.upload(icon_data).await?)));
       }
   
       if let Some(Uri::File(cover)) = meta.cover_uri() {
           let cover_data = std::fs::read(cover)?;
           meta.set_cover_uri(Some(Uri::Ipfs(ipfs_client.upload(cover_data).await?)));
       }
   
       let serialized = json!(meta).to_string();
       ipfs_client.upload(serialized).await
   }

2. Define traits for accessing and modifying URIs:

   trait HasIconUri {
       fn icon_uri(&self) -> &Option<Uri>;
       fn set_icon_uri(&mut self, uri: Option<Uri>);
   }
   
   trait HasCoverUri {
       fn cover_uri(&self) -> &Option<Uri>;
       fn set_cover_uri(&mut self, uri: Option<Uri>);
   }

3. Implement the traits for your metadata structs:

   impl HasIconUri for WorldMetadata {
       fn icon_uri(&self) -> &Option<Uri> {
           &self.icon_uri
       }
   
       fn set_icon_uri(&mut self, uri: Option<Uri>) {
           self.icon_uri = uri;
       }
   }
   
   impl HasCoverUri for WorldMetadata {
       fn cover_uri(&self) -> &Option<Uri> {
           &self.cover_uri
       }
   
       fn set_cover_uri(&mut self, uri: Option<Uri>) {
           self.cover_uri = uri;
       }
   }
   
   impl HasIconUri for ResourceMetadata {
       fn icon_uri(&self) -> &Option<Uri> {
           &self.icon_uri
       }
   
       fn set_icon_uri(&mut self, uri: Option<Uri>) {
           self.icon_uri = uri;
       }
   }
   
   // ResourceMetadata may not have a cover_uri; implement accordingly

4. Update the upload methods:

   #[async_trait]
   impl MetadataStorage for WorldMetadata {
       async fn upload(&self) -> Result<String> {
           let mut meta = self.clone();
           upload_metadata(&mut meta).await
       }
   }
   
   #[async_trait]
   impl MetadataStorage for ResourceMetadata {
       async fn upload(&self) -> Result<String> {
           let mut meta = self.clone();
           upload_metadata(&mut meta).await
       }
   }

   Adjust the traits and helper functions based on the specific fields each metadata struct contains.

Also applies to: 67-81

---

48-48: Assess the impact of cloning large metadata structures

Ohayo, sensei! Cloning self to create meta can be costly if the metadata structures are large. Consider optimizing to clone only the necessary parts or borrow mutably if possible.

Example modification:

• Instead of cloning the entire object, create a new instance with updated fields or refactor methods to work with references where appropriate.

Also applies to: 69-69

---

52-60: Handle potential errors when reading files and uploading to IPFS

Ohayo, sensei! When reading files and uploading data to IPFS, consider adding more robust error handling or user-friendly error messages to assist in debugging if file reading fails or upload issues occur.

Example:

• Wrap file reading and uploading in a custom function that provides context-specific error messages.

Also applies to: 73-76

crates/dojo/world/src/metadata/metadata_test.rs (2)

21-23: Sensei, consider using crate-relative paths to ensure test data is found

Using relative paths like "./src/metadata/metadata_test_data/cover.png" may cause tests to fail if they're run from a different working directory. Ohayo! To make the tests more robust, you might use env!("CARGO_MANIFEST_DIR") to build paths relative to the crate root.

You can modify the path construction as follows:

-PathBuf::from_str("./src/metadata/metadata_test_data/cover.png").unwrap()
+PathBuf::from(format!(
+    "{}/src/metadata/metadata_test_data/cover.png",
+    env!("CARGO_MANIFEST_DIR")
+))

Also applies to: 45-47

---

137-137: Consider mocking IpfsClient for tests

Ohayo, sensei! The TODO comment suggests that it would be beneficial to fake or mock the IpfsClient for tests to avoid actual IPFS calls and improve test reliability and speed.

Would you like assistance in implementing a mock IpfsClient for your tests?

crates/sozo/ops/src/migrate/mod.rs (2)

172-189: Avoid unnecessary cloning of item

Ohayo, sensei! To improve efficiency, consider passing a reference to item instead of cloning it when creating new_metadata.

Apply this diff:

-        let new_metadata = metadata_config::ResourceMetadata::from(item.clone());
+        let new_metadata = metadata_config::ResourceMetadata::from(item);

---

154-155: Simplify error handling by avoiding string conversion

Ohayo, sensei! Instead of converting errors to strings and then wrapping them with anyhow!, you can use anyhow::Error::from to preserve error context.

Apply this diff:

-        invoker.multicall().await.map_err(|e| anyhow!(e.to_string()))?;
+        invoker.multicall().await.map_err(anyhow::Error::from)?;

And similarly later:

-        invoker.invoke_all_sequentially().await.map_err(|e| anyhow!(e.to_string()))?;
+        invoker.invoke_all_sequentially().await.map_err(anyhow::Error::from)?;

Also applies to: 160-161

Configuration used: .coderabbit.yaml
Review profile: CHILL

#!/bin/bash
# Description: Verify the ResourceConfig usage across the codebase
# Expected: Find usage in profile_config.rs and metadata handling code

# Check for ResourceConfig usage
echo "Checking ResourceConfig usage..."
rg -p "ResourceConfig" --type rust

# Check for relationship with metadata handling
echo "Checking metadata-related code..."
rg -p "metadata" --type rust --glob "crates/dojo/world/src/**"

#!/bin/bash
# Search for references to the world address
rg "0x52ee4d3cba58d1a0462bbfb6813bf5aa1b35078c3b859cded2b727c1d9469ea"

# Check for any world initialization or metadata upload scripts
fd -g "*world*.{sh,rs,cairo}" --exec grep -l "metadata"

#!/bin/bash
# Description: Verify that the metadata feature is being used in the codebase

# Search for metadata-related functionality usage
rg -l "use.*dojo_world.*metadata" 

# Search for potential metadata feature gates
rg "#\[cfg\(feature.*metadata\)\]"

#!/bin/bash
# Let's verify the metadata feature usage in dojo-world and sozo
# First, check for metadata-related imports and usage in sozo
rg -A 5 "use dojo_world::metadata" crates/sozo/

# Then check for metadata-related functionality in sozo
rg -A 5 "metadata" crates/sozo/ops/src/

#!/bin/bash
# Description: Check for any remaining IPFS-related functionality that might be affected

# Search for IPFS-related imports or usage
rg -i "ipfs|ipfs_api"

# Look for any test files that might need the IPFS backend
fd -e rs test_ --exec rg -l "ipfs"

#!/bin/bash
# Let's check if sozo-ops has any direct IPFS functionality or tests that might be affected

# Check for IPFS-related code in sozo-ops
rg -i "ipfs" crates/sozo/ops/ -A 5

# Check if sozo-ops has any dependencies on IPFS-related crates in its dependency tree
cargo tree -p sozo-ops | rg -i "ipfs"

# Check if there are any IPFS-related imports in sozo-ops
ast-grep --pattern 'use $_ :: $$$ipfs$$$'

#!/bin/bash
# Description: Check for metadata_hash usage in related files
# Expected: Find references in events_to_remote.rs and other related files

echo "Checking metadata_hash usage in related files..."
rg -l "metadata_hash" ./crates/dojo/world/src/

echo "Checking for potential metadata update handlers..."
ast-grep --pattern 'fn $name($$$) {
    $$$
    metadata_hash
    $$$
}'

#!/bin/bash
# Let's check the actual usage context of metadata_hash in these files

echo "Checking metadata_hash usage in remote/resource.rs..."
rg "metadata_hash" ./crates/dojo/world/src/remote/resource.rs -B 2 -A 2

echo -e "\nChecking metadata_hash usage in metadata/mod.rs..."
rg "metadata_hash" ./crates/dojo/world/src/metadata/mod.rs -B 2 -A 2

echo -e "\nChecking metadata_hash usage in remote/events_to_remote.rs..."
rg "metadata_hash" ./crates/dojo/world/src/remote/events_to_remote.rs -B 2 -A 2

echo -e "\nChecking metadata_hash usage in diff/resource.rs..."
rg "metadata_hash" ./crates/dojo/world/src/diff/resource.rs -B 2 -A 2

echo -e "\nChecking metadata_hash usage in diff/compare.rs..."
rg "metadata_hash" ./crates/dojo/world/src/diff/compare.rs -B 2 -A 2

#!/bin/bash
# Description: Search for error handling patterns in metadata upload scenarios
# Expected: Find test cases covering metadata upload failures and recovery

# Look for test cases related to metadata upload failures
rg -A 10 "upload_metadata.*fail|fail.*upload_metadata" --type rust

# Look for error recovery scenarios in tests
ast-grep --pattern 'fn test_$_metadata_upload($_) {
  $$$
}'

#!/bin/bash
# Search for metadata upload implementation and error handling
ast-grep --pattern 'impl $_ {
  $$$
  upload_metadata($$$) {
    $$$
  }
  $$$
}'

# Look for retry mechanisms or error recovery patterns
rg -A 5 "retry|recover|fallback" --type rust

# Find test cases that might cover metadata upload
rg -A 10 "test.*metadata|metadata.*test" --type rust

#!/bin/bash
# Description: Verify MetadataUpdate event structure in the world contract
ast-grep --pattern 'enum Event {
  $$$
  MetadataUpdate: MetadataUpdate,
  $$$
}'

#!/bin/bash
# Let's try a different approach to find the event structure
rg -A 5 "struct MetadataUpdate" 

#!/bin/bash
# Also search for the event enum definition with a more flexible pattern
rg -A 10 "enum Event \{"

#!/bin/bash
# Search for files that might contain the event definition
fd "world\.cairo$"

#!/bin/bash
# Let's check the world contract for MetadataUpdate event
rg -A 5 "MetadataUpdate" crates/dojo/core/src/world/world_contract.cairo

#!/bin/bash
# Also check the event struct definition
rg -B 5 -A 5 "struct MetadataUpdate" crates/dojo/core/src/world/world_contract.cairo

#!/bin/bash
# Check for metadata upload implementation that would use this hash
echo "Searching for metadata upload implementation..."
rg -l "metadata_hash" --type rust

# Look for any tests involving metadata hash comparison
echo "Searching for related tests..."
rg -l "test.*metadata.*hash" --type rust

#!/bin/bash
# Let's check how metadata_hash is used in the implementation files
echo "Checking metadata_hash usage in remote/resource.rs..."
rg "metadata_hash" -B 2 -A 2 crates/dojo/world/src/remote/resource.rs

echo "\nChecking metadata_hash usage in metadata/mod.rs..."
rg "metadata_hash" -B 2 -A 2 crates/dojo/world/src/metadata/mod.rs

echo "\nChecking metadata_hash usage in migrate/mod.rs..."
rg "metadata_hash" -B 2 -A 2 crates/sozo/ops/src/migrate/mod.rs

# Let's also check for any metadata comparison logic
echo "\nChecking for metadata comparison implementations..."
ast-grep --pattern 'fn $FUNC_NAME($$$) {
    $$$
    metadata_hash
    $$$
}'

#!/bin/bash
# Search for other error handling patterns in migration-related code
rg -A 2 "expect|unwrap|context" crates/sozo/ops/src/

#!/bin/bash
# Search for error handling in migration.rs and related files
rg -A 5 "migrate_spawn_and_move|MigrationResult" crates/sozo/ops/src/

#!/bin/bash
# Search for namespace metadata references
rg -A 3 'namespace.*metadata' 

#!/bin/bash
# Search for namespace struct definitions and implementations
ast-grep --pattern 'struct Namespace {
  $$$
}'

# Also check for NamespaceLocal struct
ast-grep --pattern 'struct NamespaceLocal {
  $$$
}'

# Look for any metadata fields in these structs
rg -B 2 -A 5 'struct (Namespace|NamespaceLocal)'

#!/bin/bash
# Description: Check for test coverage of the new metadata hash methods
# Look for test files that exercise these methods

# Search for test files that might contain metadata hash related tests
echo "=== Checking for metadata hash tests ==="
rg -l "metadata_hash" "$(fd -t f -e rs test)"

# Look for specific test cases
echo "=== Checking for specific test cases ==="
rg "set_metadata_hash|metadata_hash" "$(fd -t f -e rs test)"

#!/bin/bash
# Description: Check for potential uninitialized metadata hash cases
# Look for places where WorldRemote is constructed without setting metadata_hash

# Search for WorldRemote struct construction
ast-grep --pattern 'WorldRemote {
  $$$
  metadata_hash: $_,
  $$$
}'

# Search for potential metadata_hash assignments
rg -A 3 'metadata_hash\s*=' 

#!/bin/bash
# Check for WorldRemote struct definition and initialization
ast-grep --pattern 'struct WorldRemote {
  $$$
}'

# Look for metadata_hash field initialization in constructors/new functions
ast-grep --pattern 'fn new($$$) -> WorldRemote {
  $$$
}'

# Search for any direct metadata_hash field access
rg -A 2 '\.metadata_hash'

#!/bin/bash
# Description: Check for consistent usage of metadata hash fields
# Expected: All metadata-related code should handle the hash fields

# Search for metadata-related structs and their usage
rg -A 5 "struct.*Metadata|fn.*metadata" --type rust

# Search for hash field usage in metadata context
rg -A 3 "metadata.*hash|hash.*metadata" --type rust

crates/dojo/world/src/metadata/mod.rs (1)

27-45: Enhance error handling with specific error types

Ohayo! While using anyhow::Result is convenient, it might be better to define specific error types for different failure scenarios:

• IPFS upload failures
• File reading errors
• Hash computation errors

Consider creating a custom error enum:

#[derive(Debug, thiserror::Error)]
pub enum MetadataError {
    #[error("Failed to upload to IPFS: {0}")]
    IpfsUpload(#[from] ipfs::Error),
    #[error("Failed to read file: {0}")]
    FileRead(#[from] std::io::Error),
    #[error("Hash computation failed: {0}")]
    HashComputation(String),
}

pub type Result<T> = std::result::Result<T, MetadataError>;

crates/dojo/world/src/config/metadata_config.rs (1)

39-96: Ohayo, sensei! Consider implementing a common trait for file hashing

Since both WorldMetadata and ResourceMetadata share similar file hashing logic, consider extracting this functionality into a common trait to promote code reuse and ensure consistent error handling across all implementations.

Example trait design:

trait FileHasher {
    fn hash_file<H: Hasher>(&self, path: &std::path::Path, state: &mut H) {
        if let Ok(data) = std::fs::read(path) {
            data.hash(state);
        } else {
            eprintln!("Warning: Unable to read file for hashing: {}", path.display());
        }
    }
}

crates/sozo/ops/src/tests/migration.rs (3)

42-42: Consider enhancing error handling in metadata upload.

Ohayo sensei! While the implementation looks good, the error messages could be more descriptive to help with debugging.

Consider this enhancement:

-    let res = migration.migrate(&mut ui).await.expect("Migration spawn-and-move failed.");
+    let res = migration.migrate(&mut ui).await
+        .context("Failed to migrate spawn-and-move world")?;

     if with_metadata {
-        migration.upload_metadata(&mut ui).await.expect("Upload metadata failed");
+        migration.upload_metadata(&mut ui).await
+            .context("Failed to upload world metadata after successful migration")?;
     }

Also applies to: 63-69

---

89-111: Add documentation for the check_resources helper function.

Ohayo! The helper function is well-implemented, but it would benefit from documentation explaining:

• Purpose and usage
• Parameter descriptions
• Expected behavior of the checker function

Consider adding this documentation:

+/// Validates metadata for a list of resources against expected criteria
+/// 
+/// # Arguments
+/// * `diff` - The WorldDiff containing the resources to check
+/// * `resources` - Optional list of ResourceConfig to validate
+/// * `expected_count` - Expected number of resources
+/// * `checker` - Function to validate the metadata hash of each resource
 fn check_resources(
     diff: &WorldDiff,
     resources: Option<Vec<ResourceConfig>>,
     expected_count: usize,
     checker: &dyn Fn(Felt) -> bool,
 ) {

---

113-150: Test implementation is thorough but could be more explicit.

Ohayo sensei! The test covers the key scenarios well, but consider these enhancements:

1. Add explicit assertions for specific metadata values, not just presence/absence
2. Test error cases (e.g., failed uploads)

Also, the test aligns perfectly with the PR objectives mentioned in #2096, ensuring metadata can be uploaded after initial deployment.

crates/dojo/world/src/metadata/metadata_test.rs (2)

14-64: Ohayo! Consider improving test data handling, sensei.

The helper functions could be more robust with these improvements:

1. Extract hardcoded file paths into constants
2. Add validation for social media URLs
3. Consider using a test fixture or configuration file for test data

Here's a suggested improvement:

+ const TEST_DATA_DIR: &str = "./src/metadata/metadata_test_data";
+ const TEST_COVER_PATH: &str = "./src/metadata/metadata_test_data/cover.png";
+ const TEST_ICON_PATH: &str = "./src/metadata/metadata_test_data/icon.png";

fn build_world_metadata() -> WorldMetadata {
-    cover_uri: Some(Uri::File(
-        fs::canonicalize(
-            PathBuf::from_str("./src/metadata/metadata_test_data/cover.png").unwrap(),
-        )
-        .unwrap(),
-    )),
+    cover_uri: Some(Uri::File(
+        fs::canonicalize(PathBuf::from_str(TEST_COVER_PATH).unwrap())
+            .expect("Test cover image not found")
+    )),

---

66-138: Ohayo! Great test coverage, but let's make it even better, sensei!

The test thoroughly validates the happy path, but consider adding:

1. Negative test cases (invalid URIs, network failures)
2. Edge cases (empty fields, very large metadata)

Regarding the TODO on line 137:

Would you like me to help implement a mock IpfsClient for more reliable testing? This would:

• Remove external IPFS dependency
• Allow testing network failure scenarios
• Speed up test execution

crates/dojo/world/src/diff/compare.rs (1)

Line range hint 19-29: Ohayo! Critical issue: Metadata changes are not detected in comparison logic

The comparison implementations for ContractLocal, ModelLocal, and EventLocal only check class_hash but ignore metadata_hash changes. This doesn't align with the PR's objective of properly handling metadata updates.

Here's how to fix the ContractLocal implementation (apply similar changes to ModelLocal and EventLocal):

 impl ComparableResource for ContractLocal {
     fn compare(self, remote: ResourceRemote) -> ResourceDiff {
         let remote_contract = remote.as_contract_or_panic();
 
-        if self.common.class_hash == remote_contract.common.current_class_hash() {
+        if self.common.class_hash == remote_contract.common.current_class_hash() 
+            && self.common.metadata_hash == remote_contract.common.metadata_hash {
             ResourceDiff::Synced(ResourceLocal::Contract(self), remote)
         } else {
             ResourceDiff::Updated(ResourceLocal::Contract(self), remote)
         }
     }
 }

Also applies to: 30-40, 41-51

crates/dojo/world/src/config/profile_config.rs (2)

21-23: Consider adding field-level documentation, sensei!

While the struct is well-documented, adding field-level documentation for the new metadata fields would help users understand their purpose and usage better.

Example addition:

 #[derive(Debug, Clone, Default, Deserialize)]
 pub struct ProfileConfig {
     pub world: WorldConfig,
+    /// Metadata configurations for models in the world
     pub models: Option<Vec<ResourceConfig>>,
+    /// Metadata configurations for contracts in the world
     pub contracts: Option<Vec<ResourceConfig>>,
+    /// Metadata configurations for events in the world
     pub events: Option<Vec<ResourceConfig>>,

---

212-231: Consider adding edge case tests, sensei!

While the happy path is well tested, consider adding tests for:

• Empty vectors for models/contracts/events
• Invalid URI formats
• Duplicate tags within the same resource type

Example test case:

#[test]
fn test_profile_config_empty_resources() {
    let content = r#"
    [world]
    name = "test"
    seed = "abcd"

    [namespace]
    default = "test"

    [[models]]
    [[contracts]]
    [[events]]
    "#;

    let config = toml::from_str::<ProfileConfig>(content).unwrap();
    assert!(config.models.unwrap().is_empty());
    assert!(config.contracts.unwrap().is_empty());
    assert!(config.events.unwrap().is_empty());
}

crates/sozo/ops/src/migrate/mod.rs (2)

109-164: Ohayo, sensei! Ensure calls exist before invoking

To prevent unnecessary operations when there are no metadata changes, consider checking if there are any invocations before performing multicall or sequential calls. This enhances efficiency and avoids redundant actions.

Apply this diff to add the check:

     if !invoker.calls.is_empty() {
+        if self.do_multicall() {
             ui.update_text_boxed(format!("Uploading {} metadata...", invoker.calls.len()));
             invoker.multicall().await.map_err(|e| anyhow!(e.to_string()))?;
         } else {
             ui.update_text_boxed(format!(
                 "Uploading {} metadata (sequentially)...",
                 invoker.calls.len()
             ));
             invoker.invoke_all_sequentially().await.map_err(|e| anyhow!(e.to_string()))?;
         }
+    } else {
+        ui.update_text("No metadata changes detected. Skipping upload.");
+    }

---

166-192: Ohayo, sensei! Parallelize metadata uploads for better performance

The metadata uploads in upload_metadata_from_resource_config are currently processed sequentially. To optimize performance, especially with numerous resources, consider parallelizing the uploads using asynchronous tasks.

You can refactor the function to process uploads concurrently:

async fn upload_metadata_from_resource_config(
    &self,
    config: &[ResourceConfig],
) -> anyhow::Result<Vec<Call>> {
    let futures = config.iter().map(|item| async {
        let selector = dojo_types::naming::compute_selector_from_tag_or_name(&item.tag);
        let current_hash = self
            .diff
            .resources
            .get(&selector)
            .map_or(Felt::ZERO, |r| r.metadata_hash());
        let new_metadata = metadata_config::ResourceMetadata::from(item.clone());
        let res = new_metadata.upload_if_changed(current_hash).await?;

        if let Some((new_uri, new_hash)) = res {
            Ok(Some(self.world.set_metadata_getcall(&ResourceMetadata {
                resource_id: selector,
                metadata_uri: ByteArray::from_string(&new_uri)?,
                metadata_hash: new_hash,
            })))
        } else {
            Ok(None)
        }
    });

    let results = futures::future::join_all(futures).await;
    let mut calls = Vec::new();

    for result in results {
        if let Ok(Some(call)) = result {
            calls.push(call);
        }
    }

    Ok(calls)
}

Configuration used: .coderabbit.yaml
Review profile: CHILL

async fn upload_file_to_ipfs(
    file_path: &Path,
    ipfs_client: &IpfsClient
) -> Result<String> {
    if !file_path.exists() {
        return Err(anyhow!("File not found: {}", file_path.display()));
    }
    let data = std::fs::read(file_path)?;
    let uri = ipfs_client.upload(data).await?;
    // Verify upload
    let verified_data = ipfs_client.cat(&uri).await?;
    if verified_data != data {
        return Err(anyhow!("Upload verification failed"));
    }
    Ok(uri)
}

#[async_trait]
impl MetadataStorage for ResourceMetadata {
    async fn upload(&self, ipfs_client: &IpfsClient) -> Result<String> {
        let mut meta = self.clone();

        if let Some(Uri::File(icon)) = &self.icon_uri {
            meta.icon_uri = Some(Uri::Ipfs(upload_file_to_ipfs(icon, ipfs_client).await?));
        };

        let serialized = json!(meta).to_string();
        ipfs_client.upload(serialized).await
    }
}

#!/bin/bash
# Description: Verify usage of Uri hashing in metadata-related code

# Search for metadata structs using Uri
rg --type rust "struct.*Metadata.*Uri" -C 5

# Search for metadata upload functions using Uri
rg --type rust "upload.*metadata.*Uri" -C 5

# Search for Hash derivations on metadata structs
ast-grep --pattern 'derive($$$, Hash, $$$)
struct $_Metadata'

#!/bin/bash
# Let's search for Uri usage in the codebase more broadly

# Find Uri struct definition and its usages
rg --type rust "enum Uri" -B 2 -A 5

# Search for metadata-related code that might use Uri
rg --type rust "Metadata" -B 2 -A 5

# Look for Uri serialization/deserialization usage
rg --type rust "Uri::from_string|Uri::serialize" -B 2 -A 5

# Search for tests involving Uri
rg --type rust "test.*Uri" -B 2 -A 5

#!/bin/bash
# Let's search for the Uri implementation and its usage in metadata handling

# Find Uri implementation and its methods
rg --type rust "impl.*Uri" -B 2 -A 10

# Search for Uri usage in metadata-related code
rg --type rust "Uri.*metadata" -B 2 -A 5

# Look for Uri serialization tests
rg --type rust "test.*Uri.*serialize" -B 2 -A 5

# Search for metadata hash implementations
rg --type rust "metadata_hash" -B 2 -A 5

#!/bin/bash
# Description: Check for consistent metadata handling across the codebase

# Look for metadata-related functions that should handle these new fields
rg -A 5 "metadata" --type rust

# Check for any existing metadata upload implementations
ast-grep --pattern 'fn $upload_metadata($$$) {
  $$$
}'

#!/bin/bash
# Check for metadata upload implementation in world config
ast-grep --pattern 'impl $_ {
  $$$
  pub async fn upload($$$) {
    $$$
  }
  $$$
}'

# Look for metadata storage/handling tests
rg "test.*metadata" --type rust

#!/bin/bash
# Description: Check how metadata_hash is used in metadata updates

# Search for metadata hash comparisons
rg -A 3 "metadata_hash" --type rust

# Look for metadata update implementations
ast-grep --pattern 'fn $update_name($params) {
  $$$
  metadata_hash
  $$$
}'

#!/bin/bash
# Description: Check for consistent hash computation across the codebase

# Look for hash computation implementations
ast-grep --pattern 'fn $compute_name($params) {
  $$$
  hash
  $$$
}'

# Search for direct hash assignments
rg -A 3 "metadata_hash.*=" --type rust

#!/bin/bash
# Check for hash computation implementation details

# Look for the compute_metadata_hash function implementation
ast-grep --pattern 'fn compute_metadata_hash<$_>($params) {
  $$$
}'

# Search for metadata hash computation related code
rg -A 5 "compute_metadata_hash" --type rust

# Look for any hash-related trait implementations
ast-grep --pattern 'impl Hash for $type {
  $$$
}'

crates/dojo/world/src/metadata/metadata_service.rs (1)

1-4: Ohayo! The trait definition looks solid, sensei! 🎋

The trait is well-designed with appropriate marker traits (Send + Sync + Unpin) making it safe for concurrent usage. The use of anyhow::Result provides flexible error handling which is crucial for metadata operations that might fail.

Consider documenting the trait with rustdoc comments to explain:

• The purpose of the service
• The thread-safety guarantees
• The relationship with IPFS

crates/dojo/world/src/metadata/fake_metadata_service.rs (3)

1-11: Ohayo! The imports and struct definition look solid, sensei!

The structure is clean and well-organized. However, it would be helpful to add documentation comments explaining the purpose of this fake service and its role in testing.

Consider adding documentation like this:

+/// A mock implementation of MetadataService for testing purposes.
+/// Stores metadata in memory using a HashMap instead of making actual IPFS calls.
 pub struct FakeMetadataService {
     data: HashMap<String, Vec<u8>>,
 }

---

13-17: Constructor looks good, sensei!

Consider adding the #[must_use] attribute since this function creates a new value.

 impl FakeMetadataService {
+    #[must_use]
     pub fn new() -> Self {
         Self { data: HashMap::new() }
     }
 }

---

32-35: Simplify the get() method implementation, sensei!

The current implementation can be made more idiomatic using cloned().

     #[cfg(test)]
     async fn get(&self, uri: String) -> Result<Vec<u8>> {
-        Ok(self.data.get(&uri).map(|x| x.clone()).unwrap_or(Vec::<u8>::new()))
+        Ok(self.data.get(&uri).cloned().unwrap_or_default())
     }

crates/dojo/world/src/metadata/metadata_test.rs (3)

16-52: Ohayo! Consider extracting common path handling logic, sensei.

The builder functions are well-structured, but there's repeated path handling logic. Consider creating a helper function:

fn test_file_path(filename: &str) -> PathBuf {
    fs::canonicalize(
        PathBuf::from_str(&format!("./src/metadata/metadata_test_data/{}", filename)).unwrap()
    ).unwrap()
}

Then use it in both builders:

icon_uri: Some(Uri::File(test_file_path("icon.png"))),

---

66-140: Consider adding edge cases for more robust testing, sensei.

The test has good coverage of the happy path and basic change detection. However, consider adding:

1. Test with invalid/corrupted metadata
2. Test with non-existent files
3. Test concurrent uploads of the same metadata

Example for invalid metadata test:

#[tokio::test]
async fn test_world_metadata_invalid() {
    let mut metadata_service = FakeMetadataService::new();
    let mut invalid_metadata = build_world_metadata();
    invalid_metadata.cover_uri = Some(Uri::File(PathBuf::from("/nonexistent/path")));
    
    let res = invalid_metadata
        .upload_if_changed(&mut metadata_service, Felt::ZERO)
        .await;
    
    assert!(res.is_err());
}

---

142-197: Enhance assertions for resource metadata validation, sensei.

While the test structure is solid, consider strengthening the validation by:

1. Adding assertions for the exact IPFS URI format (not just the prefix)
2. Verifying the content-type of the uploaded icon
3. Checking the size limits of the uploaded metadata

Example enhancement:

fn assert_ipfs_uri_format(uri: &str) {
    assert!(uri.starts_with("ipfs://"));
    assert_eq!(uri.len(), 46); // IPFS CIDv0 length
    assert!(uri[7..].chars().all(|c| c.is_ascii_hexdigit()));
}

crates/sozo/ops/src/migrate/mod.rs (1)

158-158: Consider enhancing error message

Ohayo! The error message could be more descriptive to help with debugging.

-            invoker.multicall().await.map_err(|e| anyhow!(e.to_string()))?;
+            invoker.multicall().await.map_err(|e| anyhow!("Failed to upload metadata via multicall: {}", e))?;

crates/dojo/world/src/metadata/ipfs_service.rs (2)

9-11: Consider Making IPFS Client Configurable

Hardcoding the IPFS client URL may limit flexibility across different environments.

Consider loading IPFS_CLIENT_URL from an environment variable or a configuration file to enhance configurability.

---

18-23: Enhance Error Handling in new Method

Errors during client initialization may lack sufficient context for debugging.

Consider adding more descriptive error messages to improve troubleshootability.

Apply this diff:

 impl IpfsMetadataService {
     pub fn new() -> Result<Self> {
         dotenv().ok();
         let client_url = env::var("IPFS_CLIENT_URL").unwrap_or_else(|_| "https://ipfs.infura.io:5001".to_string());
         let username = env::var("IPFS_USERNAME")?;
         let password = env::var("IPFS_PASSWORD")?;

-        Ok(Self {
-            client: ipfs_api_backend_hyper::IpfsClient::from_str(&client_url)?
-                .with_credentials(&username, &password),
-        })
+        let client = ipfs_api_backend_hyper::IpfsClient::from_str(&client_url)
+            .map_err(|e| anyhow::anyhow!("Failed to create IPFS client: {}", e))?
+            .with_credentials(&username, &password);

+        Ok(Self { client })
     }
 }

crates/dojo/world/src/metadata/metadata.rs (3)

12-19: Ohayo, sensei! Consider passing data by reference in compute_metadata_hash

Passing data by value might lead to unnecessary copies if T is a large type. It's more efficient to pass data as a reference.

Apply this diff to update the function signature:

- fn compute_metadata_hash<T>(data: T) -> u64
+ fn compute_metadata_hash<T>(data: &T) -> u64

And update the call site accordingly:

- let new_hash = compute_metadata_hash(self);
+ let new_hash = compute_metadata_hash(self);

---

50-58: Ohayo, sensei! Refactor duplicated code for uploading URIs

The code for uploading icon_uri and cover_uri is duplicated. Extracting a helper function can reduce repetition and improve maintainability.

For example, create a helper function:

async fn upload_uri(
    uri_option: &Option<Uri>,
    service: &mut impl MetadataService,
) -> Result<Option<Uri>> {
    if let Some(Uri::File(path)) = uri_option {
        let data = std::fs::read(path)?;
        let uploaded_uri = Uri::Ipfs(service.upload(data).await?);
        Ok(Some(uploaded_uri))
    } else {
        Ok(uri_option.clone())
    }
}

Then update the upload method:

- if let Some(Uri::File(icon)) = &self.icon_uri {
-     let icon_data = std::fs::read(icon)?;
-     meta.icon_uri = Some(Uri::Ipfs(service.upload(icon_data).await?));
- };
+ meta.icon_uri = upload_uri(&self.icon_uri, service).await?;

- if let Some(Uri::File(cover)) = &self.cover_uri {
-     let cover_data = std::fs::read(cover)?;
-     meta.cover_uri = Some(Uri::Ipfs(service.upload(cover_data).await?));
- };
+ meta.cover_uri = upload_uri(&self.cover_uri, service).await?;

---

68-77: Ohayo, sensei! Refactor duplicated code in ResourceMetadata

Similar to WorldMetadata, the code for uploading icon_uri can be refactored to use the helper function, reducing duplication.

Update the upload method in ResourceMetadata:

- if let Some(Uri::File(icon)) = &self.icon_uri {
-     let icon_data = std::fs::read(icon)?;
-     meta.icon_uri = Some(Uri::Ipfs(service.upload(icon_data).await?));
- };
+ meta.icon_uri = upload_uri(&self.icon_uri, service).await?;

Note: Ensure the helper function upload_uri is accessible to both implementations.

Configuration used: .coderabbit.yaml
Review profile: CHILL

#!/bin/bash
# Description: Find implementations of MetadataService trait
# Expected: Should find at least IpfsMetadataService and test implementations

# Search for struct/class implementations
ast-grep --pattern 'impl $_ for $_ where $$$'

# Search for direct implementations
rg "impl.*MetadataService.*for"

#!/bin/bash
# Let's check the implementations to verify they align with the trait definition
rg -B 5 -A 10 "impl MetadataService for" crates/dojo/world/src/metadata/ipfs_service.rs crates/dojo/world/src/metadata/fake_metadata_service.rs

crates/dojo/world/src/metadata/ipfs_service.rs (2)

14-19: Ohayo, sensei! Consider enhancing Debug implementation

The current Debug implementation doesn't provide any useful information. Consider including the client URL for better debugging experience.

 impl std::fmt::Debug for IpfsMetadataService {
-    fn fmt(&self, _f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
-        Ok(())
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
+        write!(f, "IpfsMetadataService {{ client_url: {} }}", self.client.base_url())
     }
 }

---

39-47: Ohayo, sensei! Improve URI validation in get method

The get method should validate the URI format before processing.

     #[cfg(test)]
     async fn get(&self, uri: String) -> Result<Vec<u8>> {
+        if !uri.starts_with("ipfs://") {
+            return Err(anyhow::anyhow!("Invalid IPFS URI format: must start with ipfs://"));
+        }
         let res = self
             .client
             .cat(&uri.replace("ipfs://", ""))
             .map_ok(|chunk| chunk.to_vec())
             .try_concat()
-            .await?;
+            .await
+            .map_err(|e| anyhow::anyhow!("Failed to retrieve from IPFS: {}", e))?;
         Ok(res)
     }

bin/sozo/src/commands/migrate.rs (1)

84-90: Excellent implementation sensei, with a minor suggestion

The metadata service integration looks solid and addresses the core objectives of uploading metadata after failed deployments. The error handling is good but could be more descriptive.

Consider enhancing the error context:

 let mut metadata_service =
-    IpfsMetadataService::new(IPFS_CLIENT_URL, IPFS_USERNAME, IPFS_PASSWORD)?;
+    IpfsMetadataService::new(IPFS_CLIENT_URL, IPFS_USERNAME, IPFS_PASSWORD)
+        .context("Failed to initialize IPFS metadata service")?;

 migration
     .upload_metadata(&mut spinner, &mut metadata_service)
     .await
-    .context("Metadata upload failed.")?;
+    .context("Failed to upload world metadata to IPFS")?;

crates/dojo/world/src/metadata/metadata_test.rs (3)

16-21: Ohayo! Consider making test file paths more robust, sensei.

The hardcoded path ./src/metadata/metadata_test_data/ might break if tests are run from different working directories.

Consider using environment variables or relative paths from the crate root:

-    fs::canonicalize(
-        PathBuf::from_str(&format!("./src/metadata/metadata_test_data/{}", filename)).unwrap(),
-    )
+    let test_data_dir = env!("CARGO_MANIFEST_DIR").to_owned() + "/src/metadata/metadata_test_data";
+    fs::canonicalize(PathBuf::from_str(&format!("{}/{}", test_data_dir, filename)).unwrap())

---

58-132: Enhance test coverage with error cases, sensei.

While the happy path is well tested, consider adding test cases for:

1. Invalid file paths
2. Malformed metadata
3. Service failures

Also, consider extracting magic values:

+ const HASH_INCREMENT: Felt = Felt::ONE;
- world_metadata.upload_if_changed(&mut metadata_service, current_hash + Felt::ONE).await
+ world_metadata.upload_if_changed(&mut metadata_service, current_hash + HASH_INCREMENT).await

---

1-189: Excellent alignment with PR objectives, sensei! 🎯

The tests thoroughly validate the core requirement of uploading metadata only when changes are detected:

• Hash-based change detection
• Proper re-upload behavior
• Comprehensive content verification

Consider adding integration tests that simulate the full deployment workflow, including failure scenarios mentioned in issue #2096.

crates/sozo/ops/src/migrate/mod.rs (2)

114-168: Consider enhancing error handling and progress tracking

Ohayo sensei! The metadata upload implementation looks solid, but could benefit from some improvements:

1. Consider adding retry logic for metadata service failures
2. Add progress tracking for large uploads (e.g., X/Y resources uploaded)

 pub async fn upload_metadata(
     &self,
     ui: &mut MigrationUi,
     service: &mut impl MetadataService,
 ) -> anyhow::Result<()> {
     ui.update_text("Uploading metadata...");
+    let total_resources = self.diff.resources.len();
+    let mut uploaded = 0;
 
     let mut invoker = Invoker::new(&self.world.account, self.txn_config);
 
     // world
     let current_hash =
         self.diff.resources.get(&Felt::ZERO).map_or(Felt::ZERO, |r| r.metadata_hash());
     let new_metadata = WorldMetadata::from(self.diff.profile_config.world.clone());
 
-    let res = new_metadata.upload_if_changed(service, current_hash).await?;
+    let res = new_metadata
+        .upload_if_changed(service, current_hash)
+        .await
+        .map_err(|e| anyhow!("Failed to upload world metadata: {}", e))?;
+    uploaded += 1;
+    ui.update_text(&format!("Uploaded {}/{} resources...", uploaded, total_resources));

---

123-154: Consider architectural optimizations for metadata handling

Ohayo sensei! The current implementation is solid, but here are some architectural suggestions to improve performance and scalability:

1. Consider implementing parallel uploads for metadata using futures::stream::StreamExt
2. Add caching layer for frequently accessed metadata to reduce IPFS calls
3. Implement batch uploads for related resources

Would you like me to provide an example implementation of these optimizations?

crates/dojo/world/src/metadata/metadata_storage.rs (2)

63-64: Optimize metadata serialization to improve efficiency

Ohayo, sensei! Instead of using json!(meta).to_string() and then converting it to bytes, you can directly serialize meta to a byte vector using serde_json::to_vec(&meta)?. This approach reduces unnecessary intermediate allocations and improves performance.

---

75-76: Optimize metadata serialization to improve efficiency

Ohayo, sensei! Similar to the previous suggestion, consider using serde_json::to_vec(&meta)? to serialize ResourceMetadata directly to a byte vector. This enhances efficiency by avoiding intermediate string conversions.

Configuration used: .coderabbit.yaml
Review profile: CHILL

#!/bin/bash
# Search for other metadata upload implementations
echo "Searching for other metadata upload implementations..."
rg -l "upload_metadata|IpfsMetadataService" --type rust

# Search for any TODO comments related to metadata
echo "Checking for metadata-related TODOs..."
rg -i "todo.*metadata" --type rust

#!/bin/bash
# Let's check the actual implementation details of metadata uploads
echo "Checking metadata upload implementations in migration module..."
rg "upload_metadata" -A 5 -B 5 crates/sozo/ops/src/migrate/mod.rs

echo "\nChecking metadata upload implementations in IPFS service..."
rg "upload_metadata" -A 5 -B 5 crates/dojo/world/src/metadata/ipfs_service.rs

echo "\nChecking test implementations..."
rg "upload_metadata" -A 5 -B 5 crates/sozo/ops/src/tests/migration.rs

-    let selector = dojo_types::naming::compute_selector_from_tag_or_name(&item.tag);
+    let selector = dojo_types::naming::compute_selector_from_tag_or_name(&item.tag)
+        .map_err(|e| anyhow!("Failed to compute selector for tag {}: {}", item.tag, e))?;

crates/dojo/world/src/metadata/ipfs_service.rs (2)

16-21: Ohayo, sensei! Consider enhancing Debug implementation

The current Debug implementation, while satisfying clippy, could be more helpful for debugging purposes. Consider including the client URL in the debug output.

 impl std::fmt::Debug for IpfsMetadataService {
-    fn fmt(&self, _f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
-        Ok(())
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
+        write!(f, "IpfsMetadataService {{ client_url: {} }}", self.client.base_url())
     }
 }

---

51-59: Ohayo, sensei! Add validation for empty data in upload method

Consider adding validation to prevent uploading empty data to IPFS.

 async fn upload(&mut self, data: Vec<u8>) -> Result<String> {
+    if data.is_empty() {
+        return Err(anyhow::anyhow!("Cannot upload empty data to IPFS"));
+    }
     let reader = Cursor::new(data);
     let response = self
         .client

crates/dojo/world/src/metadata/metadata_storage.rs (3)

39-47: Consider using streaming for large file uploads

Ohayo, sensei! While the current implementation works well for small files, loading the entire file into memory with std::fs::read could be problematic for large files. Consider implementing streaming upload support for handling large files efficiently.

 async fn upload_uri(uri: &Option<Uri>, service: &mut impl MetadataService) -> Result<Option<Uri>> {
     if let Some(Uri::File(path)) = uri {
-        let data = std::fs::read(path)?;
-        let uploaded_uri = Uri::Ipfs(service.upload(data).await?);
+        let file = tokio::fs::File::open(path).await?;
+        let uploaded_uri = Uri::Ipfs(service.upload_stream(file).await?);
         Ok(Some(uploaded_uri))
     } else {
         Ok(uri.clone())
     }
 }

---

70-87: Simplify hash comparison logic

Ohayo, sensei! The current implementation converts the hash to a Felt type for comparison, which adds unnecessary complexity. Consider using the native u64 hash value directly for comparison, as it would be more efficient and clearer.

     async fn upload_if_changed(
         &self,
         service: &mut impl MetadataService,
-        current_hash: Felt,
+        current_hash: u64,
     ) -> Result<Option<(String, u64)>> {
         let new_hash = compute_metadata_hash(self);
-        let new_hash = Felt::from_raw([0, 0, 0, new_hash]);
 
         if new_hash != current_hash {
             let new_uri = self.upload(service).await?;
             return Ok(Some((new_uri, new_hash)));
         }
 
         Ok(None)
     }

---

1-113: Consider adding monitoring capabilities

Ohayo, sensei! To better understand the system's behavior in production, consider adding monitoring capabilities:

• Track metadata upload latency
• Monitor hash computation time
• Count upload successes and failures
• Track file sizes being processed

This would help identify performance bottlenecks and potential issues early.

crates/sozo/ops/src/tests/migration.rs (3)

43-43: The migration function changes look solid, sensei!

The addition of the with_metadata parameter and conditional upload block implements the core functionality needed to address issues #2096 and #1907. The error handling is descriptive and helpful.

Consider adding a doc comment for the with_metadata parameter to clarify its purpose:

/// Migrates the spawn-and-move project from the local environment.
+ /// 
+ /// # Arguments
+ /// * `sequencer` - The runner context for the test
+ /// * `with_metadata` - When true, uploads world/models/contracts metadata after migration

Also applies to: 64-70

---

77-77: Test updates maintain existing behavior correctly!

The existing tests have been properly updated to pass false for the with_metadata parameter, ensuring they maintain their original behavior.

Consider adding negative test cases to verify that metadata is not uploaded when with_metadata is false:

assert!(is_not_set(world_diff.world_info.metadata_hash));

Also applies to: 86-89

---

91-113: Excellent helper function implementation, sensei!

The check_resources helper is well-structured and provides good reusability for metadata verification.

Consider making the error message more specific by including the expected and actual hash values:

- assert!(checker(resource.metadata_hash()), "Bad resource hash: {}", resource.name());
+ assert!(checker(resource.metadata_hash()), "Invalid metadata hash for resource {}: expected {} to be {}", 
+   resource.name(), 
+   resource.metadata_hash(),
+   if checker == is_set { "non-zero" } else { "zero" }
+ );

crates/sozo/ops/src/migrate/mod.rs (2)

114-168: Enhance error message in multicall error handling

Ohayo sensei! The implementation looks solid with good practices for metadata handling and UI feedback. However, the error message in the multicall error handling could be more descriptive.

Consider this enhancement:

-            invoker.multicall().await.map_err(|e| anyhow!(e.to_string()))?;
+            invoker.multicall().await.map_err(|e| anyhow!("Failed to upload metadata via multicall: {}", e))?;

---

170-197: Consider using early returns for cleaner flow

Ohayo sensei! The implementation is solid, but we can make it more idiomatic with early returns.

Consider this refactor:

     async fn upload_metadata_from_resource_config(
         &self,
         service: &mut impl MetadataService,
         config: &[ResourceConfig],
     ) -> anyhow::Result<Vec<Call>> {
         let mut calls = vec![];
 
         for item in config {
             let selector = dojo_types::naming::compute_selector_from_tag_or_name(&item.tag);
 
             let current_hash =
                 self.diff.resources.get(&selector).map_or(Felt::ZERO, |r| r.metadata_hash());
 
             let new_metadata = metadata_config::ResourceMetadata::from(item.clone());
 
-            let res = new_metadata.upload_if_changed(service, current_hash).await?;
-
-            if let Some((new_uri, new_hash)) = res {
-                calls.push(self.world.set_metadata_getcall(&ResourceMetadata {
-                    resource_id: selector,
-                    metadata_uri: ByteArray::from_string(&new_uri)?,
-                    metadata_hash: new_hash,
-                }));
-            }
+            let Some((new_uri, new_hash)) = new_metadata.upload_if_changed(service, current_hash).await? else {
+                continue;
+            };
+            
+            calls.push(self.world.set_metadata_getcall(&ResourceMetadata {
+                resource_id: selector,
+                metadata_uri: ByteArray::from_string(&new_uri)?,
+                metadata_hash: new_hash,
+            }));
         }
 
         Ok(calls)
     }

Configuration used: .coderabbit.yaml
Review profile: CHILL

scripts/rust_fmt.sh (1)

3-3: Consider adding documentation for the script usage

Ohayo! It would be helpful to add a comment block explaining:

• The script's purpose
• Available options (especially the new --fix flag)
• Example usage

Here's a suggested addition:

#!/bin/bash

+# Rust code formatting script
+# Usage:
+#   ./rust_fmt.sh           # Check formatting without making changes
+#   ./rust_fmt.sh --fix     # Apply formatting fixes
+#   
+# Additional cargo fmt arguments can be passed after the initial options
+
 option="--check"

examples/spawn-and-move/dojo_dev.toml (1)

6-17: Ohayo! Consider enhancing model descriptions, sensei!

While the model definitions are clear, they could benefit from more detailed descriptions including:

• Expected fields/properties
• Relationships with other models
• Usage examples

crates/dojo/world/src/config/environment.rs (2)

13-15: Ohayo! Consider enhancing security for IPFS credentials, sensei!

While the implementation follows the existing pattern, storing sensitive credentials (username/password) as plain text in a struct that derives Debug could potentially expose them in logs or error messages.

Consider:

1. Creating a dedicated type for credentials that implements custom Debug
2. Using a secure string type that zeros memory on drop

Here's a suggested approach:

use secrecy::SecretString;

#[derive(Default, Deserialize, Clone)]
pub struct IpfsCredentials {
    username: Option<String>,
    password: Option<SecretString>,
}

impl Debug for IpfsCredentials {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.debug_struct("IpfsCredentials")
            .field("username", &self.username)
            .field("password", &"[REDACTED]")
            .finish()
    }
}

---

Line range hint 13-59: Ohayo! Consider adding configuration validation, sensei!

Since this is part of the metadata upload enhancement, it might be helpful to:

1. Add a method to validate IPFS configuration completeness (all or none of the IPFS fields should be set)
2. Document the IPFS configuration requirements in the struct documentation

Here's a suggested validation method:

impl Environment {
    pub fn validate_ipfs_config(&self) -> Result<(), &'static str> {
        match (self.ipfs_url(), self.ipfs_username(), self.ipfs_password()) {
            (None, None, None) => Ok(()), // No IPFS config is valid
            (Some(_), Some(_), Some(_)) => Ok(()), // Complete IPFS config is valid
            _ => Err("Incomplete IPFS configuration: all or none of the IPFS fields must be set")
        }
    }
}

crates/dojo/world/src/metadata/metadata_storage.rs (1)

91-116: Reduce code duplication in trait implementations

Ohayo, sensei! The upload implementations for WorldMetadata and ResourceMetadata share significant code. Consider extracting the common logic into a helper function.

+async fn upload_metadata<T: serde::Serialize>(
+    meta: &mut T,
+    icon_uri: &Option<Uri>,
+    service: &mut impl MetadataService
+) -> Result<String> {
+    if let Some(new_uri) = upload_uri(icon_uri, service)
+        .await
+        .context("Failed to upload icon URI")? {
+        // Assuming there's a way to set icon_uri on T
+        meta.set_icon_uri(new_uri);
+    }
+    
+    let serialized = json!(meta).to_string();
+    service.upload(serialized.as_bytes().to_vec())
+        .await
+        .context("Failed to upload metadata")
+}

 impl MetadataStorage for WorldMetadata {
     async fn upload(&self, service: &mut impl MetadataService) -> Result<String> {
         let mut meta = self.clone();
-        meta.icon_uri =
-            upload_uri(&self.icon_uri, service).await.context("Failed to upload icon URI")?;
         meta.cover_uri =
             upload_uri(&self.cover_uri, service).await.context("Failed to upload cover URI")?;
-        let serialized = json!(meta).to_string();
-        service.upload(serialized.as_bytes().to_vec()).await.context("Failed to upload metadata")
+        upload_metadata(&mut meta, &self.icon_uri, service).await
     }
 }

bin/sozo/src/commands/migrate.rs (1)

84-105: Enhance error handling and credential validation

Ohayo sensei! While the implementation works, we can make it more robust:

1. Replace multiple unwrap() calls with safer alternatives
2. Consolidate credential validation
3. Provide more specific error messages

Consider this improvement:

-            let ipfs_url = ipfs.url(profile_config.env.as_ref());
-            let ipfs_username = ipfs.username(profile_config.env.as_ref());
-            let ipfs_password = ipfs.password(profile_config.env.as_ref());
-
-            let mut metadata_upload_text = String::new();
-
-            if ipfs_url.is_some() && ipfs_username.is_some() && ipfs_password.is_some() {
-                let mut metadata_service = IpfsMetadataService::new(
-                    &ipfs_url.unwrap(),
-                    &ipfs_username.unwrap(),
-                    &ipfs_password.unwrap(),
-                )?;
+            let mut metadata_upload_text = match (
+                ipfs.url(profile_config.env.as_ref()),
+                ipfs.username(profile_config.env.as_ref()),
+                ipfs.password(profile_config.env.as_ref()),
+            ) {
+                (Some(url), Some(username), Some(password)) => {
+                    let mut metadata_service = IpfsMetadataService::new(&url, &username, &password)
+                        .context("Failed to initialize IPFS metadata service")?;
+                    
+                    migration
+                        .upload_metadata(&mut spinner, &mut metadata_service)
+                        .await
+                        .context("Failed to upload metadata to IPFS")?;
+                    String::new()
+                }
+                _ => "\nMetadata: No IPFS credentials found => metadata upload ignored".to_string()
+            };
-                migration
-                    .upload_metadata(&mut spinner, &mut metadata_service)
-                    .await
-                    .context("Metadata upload failed.")?;
-            } else {
-                metadata_upload_text = "\nMetadata: No IPFS credentials has been found => \
-                                        metadata upload has been ignored."
-                    .to_string();
-            };

crates/sozo/ops/src/tests/migration.rs (1)

91-113: Consider adding documentation for the helper function, sensei!

The check_resources helper is well-structured with good assertions and error messages. However, it would benefit from documentation explaining:

• The purpose of the checker callback
• The expected format of resources
• The meaning of expected_count

Consider adding a doc comment like this:

+/// Validates resource metadata in a WorldDiff against expected criteria
+/// 
+/// # Arguments
+/// * `diff` - The WorldDiff containing resources to validate
+/// * `resources` - Optional vector of ResourceConfig to check
+/// * `expected_count` - Expected number of resources
+/// * `checker` - Callback to validate the metadata hash of each resource
 fn check_resources(

bin/sozo/src/commands/options/ipfs.rs (3)

9-27: Ohayo! Consider enhancing security for sensitive fields.

While the implementation looks solid, sensei, we should consider adding a security note in the help text for the password field to warn users about command-line visibility.

     #[arg(long, env = IPFS_PASSWORD_ENV_VAR)]
     #[arg(value_name = "PASSWORD")]
-    #[arg(help = "The IPFS password.")]
+    #[arg(help = "The IPFS password. Note: Prefer using environment variables for passwords to avoid shell history exposure.")]
     #[arg(global = true)]
     pub ipfs_password: Option<String>,

---

29-74: Consider refactoring to reduce duplication and enhance security.

The implementation looks good but has two areas for improvement:

1. The three methods follow the same pattern and could be refactored to reduce duplication
2. The password tracing at line 64 might expose sensitive information in logs

Consider implementing a generic method to handle the common pattern:

impl IpfsOptions {
    fn get_value<T: Clone>(
        &self,
        cmd_value: &Option<T>,
        env_value: Option<&Environment> -> Option<String>,
        field_name: &str,
    ) -> Option<String> {
        trace!("Retrieving {} for IpfsOptions.", field_name);

        if let Some(value) = cmd_value {
            trace!("Using IPFS {} from command line.", field_name);
            Some(value.to_string())
        } else if let Some(value) = env_value {
            trace!("Using IPFS {} from environment metadata.", field_name);
            Some(value)
        } else {
            trace!("No default IPFS {}.", field_name);
            None
        }
    }
}

Also, consider masking the password in trace logs:

-            trace!(?password, "Using IPFS password from command line.");
+            trace!("Using IPFS password from command line. [REDACTED]");

---

76-170: Excellent test coverage, sensei! Consider adding edge cases.

The test suite is comprehensive and well-structured. To make it even more robust, consider adding tests for:

1. Invalid URL formats
2. Empty string values
3. Unicode characters in username/password

Example test case:

#[test]
fn handles_invalid_url_format() {
    let cmd = Command::parse_from([
        "sozo",
        "--ipfs-url",
        "not_a_valid_url",
    ]);
    assert!(cmd.options.url(None).is_none());
}

crates/sozo/ops/src/migrate/mod.rs (2)

114-168: Consider adding progress tracking and retry mechanism for robustness

Ohayo sensei! The metadata upload implementation looks solid, but could benefit from these enhancements:

1. Progress tracking for large uploads (e.g., number of successful/failed uploads)
2. Retry mechanism for failed uploads to handle temporary network issues

 pub async fn upload_metadata(
     &self,
     ui: &mut MigrationUi,
     service: &mut impl MetadataService,
 ) -> anyhow::Result<()> {
     ui.update_text("Uploading metadata...");
+    let mut success_count = 0;
+    let mut retry_count = 0;
+    const MAX_RETRIES: u32 = 3;
 
     let mut invoker = Invoker::new(&self.world.account, self.txn_config);
 
     // world
     let current_hash =
         self.diff.resources.get(&Felt::ZERO).map_or(Felt::ZERO, |r| r.metadata_hash());
     let new_metadata = WorldMetadata::from(self.diff.profile_config.world.clone());
 
-    let res = new_metadata.upload_if_changed(service, current_hash).await?;
+    let res = retry_with_backoff(MAX_RETRIES, || {
+        retry_count += 1;
+        new_metadata.upload_if_changed(service, current_hash)
+    }).await?;
 
     if let Some((new_uri, new_hash)) = res {
         invoker.add_call(self.world.set_metadata_getcall(&ResourceMetadata {
@@ -133,6 +137,7 @@
             metadata_uri: ByteArray::from_string(&new_uri)?,
             metadata_hash: new_hash,
         }));
+        success_count += 1;
     }

---

170-197: Consider batch processing optimization for resource configs

Ohayo sensei! The current implementation processes each resource config sequentially. For large configurations, consider implementing batch processing to improve performance.

 async fn upload_metadata_from_resource_config(
     &self,
     service: &mut impl MetadataService,
     config: &[ResourceConfig],
 ) -> anyhow::Result<Vec<Call>> {
     let mut calls = vec![];
+    const BATCH_SIZE: usize = 50;
 
-    for item in config {
+    for batch in config.chunks(BATCH_SIZE) {
+        let futures: Vec<_> = batch.iter().map(|item| async {
+            let selector = dojo_types::naming::compute_selector_from_tag_or_name(&item.tag)
+                .map_err(|e| anyhow!("Failed to compute selector for tag {}: {}", item.tag, e))?;
 
-        let selector = dojo_types::naming::compute_selector_from_tag_or_name(&item.tag);
+            let current_hash =
+                self.diff.resources.get(&selector).map_or(Felt::ZERO, |r| r.metadata_hash());
 
-        let current_hash =
-            self.diff.resources.get(&selector).map_or(Felt::ZERO, |r| r.metadata_hash());
+            let new_metadata = metadata_config::ResourceMetadata::from(item.clone());
+            let res = new_metadata.upload_if_changed(service, current_hash).await?;
 
-        let new_metadata = metadata_config::ResourceMetadata::from(item.clone());
-
-        let res = new_metadata.upload_if_changed(service, current_hash).await?;
-
-        if let Some((new_uri, new_hash)) = res {
-            calls.push(self.world.set_metadata_getcall(&ResourceMetadata {
+            Ok::<Option<Call>, anyhow::Error>(if let Some((new_uri, new_hash)) = res {
+                Some(self.world.set_metadata_getcall(&ResourceMetadata {
                 resource_id: selector,
                 metadata_uri: ByteArray::from_string(&new_uri)?,
                 metadata_hash: new_hash,
-            }));
+                }))
+            } else {
+                None
+            })
+        }).collect();
+
+        let results = futures::future::join_all(futures).await;
+        for result in results {
+            if let Some(call) = result? {
+                calls.push(call);
+            }
         }
     }
 
     Ok(calls)
 }

examples/spawn-and-move/manifest_dev.json (1)

95-98: Ohayo! Excellent addition of metadata hash tracking.

The new metadata_hash field in ResourceMetadata and corresponding hash field in MetadataUpdate event directly support the PR's objective of improved metadata handling. This will enable detecting when metadata content has actually changed, ensuring uploads happen only when necessary.

This change provides a robust foundation for:

• Detecting actual changes in metadata content
• Preventing unnecessary uploads
• Tracking metadata versions

Also applies to: 1009-1013